Skip to content

Local testing fixes#89

Merged
mdellabitta merged 3 commits intomainfrom
local-testing-fixes
Mar 31, 2025
Merged

Local testing fixes#89
mdellabitta merged 3 commits intomainfrom
local-testing-fixes

Conversation

@mdellabitta
Copy link
Contributor

@mdellabitta mdellabitta commented Mar 31, 2025 •
edited by ellipsis-dev bot
Loading

Important

Adds Docker setup for Elasticsearch and PostgreSQL, database initialization scripts, and updates AWS certificate script for local testing.

  • Docker Setup:
    • Adds docker-compose.yml to set up Elasticsearch, PostgreSQL, and API services for local testing.
    • Configures Elasticsearch with a single-node setup and PostgreSQL with initial database scripts.
  • Database Initialization:
    • Adds 00-init.sql to create account table with necessary fields and constraints.
    • Adds 01-api-key.sql to insert a test API key into the account table.
  • AWS Certificate Script:
    • Modifies aws-certs.sh to use set -euxo pipefail for better error handling.
    • Updates certificate processing logic to use a specific directory for temporary files.

This description was created by Ellipsis for 97aa73f. It will automatically update as commits are pushed.

@gitguardian
Copy link

gitguardian bot commented Mar 31, 2025 •
edited
Loading

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

ellipsis-dev[bot]
ellipsis-dev bot reviewed Mar 31, 2025
View reviewed changes
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 97aa73f in 2 minutes and 36 seconds

More details
  • Looked at 667 lines of code in 5 files
  • Skipped 1 files when reviewing.
  • Skipped posting 13 drafted comments based on config settings.
1. aws-certs.sh:17
  • Draft comment:
    Consider using pushd/popd and quoting paths for reliability.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
2. aws-certs.sh:20
  • Draft comment:
    Quote variable references (e.g. "$CERT") in the loop to prevent word splitting.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. docker-compose.yml:47
  • Draft comment:
    Consolidate duplicate environment variables to reduce misconfiguration risk.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The comment points out a real issue - having duplicate environment variables across services can lead to maintenance problems and inconsistencies. However, these variables serve different purposes: the postgres service uses them for initialization, while the api service needs them for connection. The elasticsearch URLs are also distinct with different purposes (different indices/aliases). The suggestion might actually make things less clear.
    The duplication might be intentional for clarity and separation of concerns. Also, consolidating might make the configuration less explicit and harder to understand.
    While there is some duplication, it's not necessarily harmful - it makes each service's configuration self-contained and explicit. The values are consistent between services, suggesting it's well-managed.
    The comment should be deleted as the current structure, while containing some duplication, is clear and maintainable. The suggested consolidation might not actually improve the configuration.
4. aws-certs.sh:17
  • Draft comment:
    Consider quoting directory and file name variables (e.g. in 'cd' and redirection) and using pushd/popd for clarity to avoid issues if paths contain spaces.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
5. aws-certs.sh:21
  • Draft comment:
    Quote file path variables (e.g. use "$CERT") when passing them to commands like openssl to prevent word-splitting issues.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
6. db-scripts/00-init.sql:1
  • Draft comment:
    Consider adding IF NOT EXISTS clauses in CREATE statements to handle re-execution gracefully in development environments.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
7. db-scripts/01-api-key.sql:8
  • Draft comment:
    Add a newline at the end of the file to comply with POSIX standards.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the comment is technically correct about POSIX standards, this is a very minor issue. Most modern SQL databases and tools handle files without trailing newlines just fine. This feels like the kind of nitpicky comment that adds noise without providing much value. The code will work perfectly fine either way.
    The missing newline could potentially cause issues with some Unix tools or shell scripts that process these SQL files. Some SQL tools do expect proper line endings.
    While true, if this was a real issue it would likely be caught by linters or the build process. This is too minor to warrant a manual review comment.
    This comment should be removed as it's too minor and would typically be handled by automated tools rather than manual review.
8. docker-compose.yml:1
  • Draft comment:
    Add a version field (e.g. 'version: "3.8"') at the top of the file for clarity and forward compatibility.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
9. docker-compose.yml:35
  • Draft comment:
    Avoid storing credentials in plain text; consider using environment variable substitution or Docker secrets for improved security in production.
  • Reason this comment was not posted:
    Marked as duplicate.
10. es-scripts/init-indices.sh:2
  • Draft comment:
    Consider adding a timeout or maximum retry count to the 'until' loop to avoid infinite waiting if Elasticsearch never becomes available.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While infinite loops can be problematic, this is a container initialization script where waiting indefinitely for a dependency is actually a common and valid pattern. The script needs Elasticsearch to be available before proceeding. If Elasticsearch never comes up, the container should fail rather than proceed with partial initialization. Adding a timeout would mean the container could start without proper index setup.
    The comment raises a valid concern about infinite loops in general. In some contexts, especially user-facing applications, having a timeout would be important.
    However, this is a container initialization script where waiting for dependencies is a standard pattern. Adding a timeout could lead to partially initialized containers which is worse than failing to start.
    The infinite wait is actually the correct pattern here for container initialization. The comment should be deleted as it suggests changing correct behavior.
11. es-scripts/init-indices.sh:12
  • Draft comment:
    Note that 'include_type_name=true' is deprecated in newer Elasticsearch versions; plan for updating the mapping when upgrading ES.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, as it only notes the deprecation of a feature in Elasticsearch without suggesting any specific action or change to the code. It doesn't ask for confirmation or suggest a code improvement.
12. db-scripts/01-api-key.sql:8
  • Draft comment:
    The file is missing a newline at the end. Adding a newline at the end of the file is a good practice to ensure compatibility and to prevent potential issues with automated tools or version control diffs.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While the comment is technically correct, this is a very minor issue that would be automatically handled by most IDEs and git tools. It doesn't affect the functionality of the SQL script. Most CI systems would catch this automatically. The rules specifically say not to comment on obvious or unimportant issues.
    The missing newline could potentially cause issues with some tools, and it is a widely accepted best practice to end files with newlines.
    While true, this is exactly the kind of minor issue that should be handled by automated tools rather than manual review comments. It doesn't affect the actual functionality.
    This comment should be removed as it's too minor and would typically be handled by automated tools.
13. docker-compose.yml:63
  • Draft comment:
    The file does not end with a newline. Please add a newline at the end of the file for consistency with common file formatting standards.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While having a newline at the end of files is a common convention and some tools expect it, this is a very minor issue that would likely be automatically handled by most editors. It's not a significant enough issue to warrant a PR comment, as it doesn't affect functionality and is more of a formatting preference.
    The comment is technically correct about common file formatting standards. Missing newlines can cause issues with some Unix tools and make diffs less clean.
    While technically valid, this is too minor of an issue to warrant a PR comment. Many editors automatically add trailing newlines, and this doesn't affect the functionality of the docker-compose file.
    Delete this comment as it's too minor of an issue and doesn't affect functionality. This falls under the rule of not making obvious or unimportant comments.

Workflow ID: wflow_RTW1lec3NR4y0Npk

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@mdellabitta mdellabitta merged commit 0487909 into main Mar 31, 2025
2 checks passed
@mdellabitta mdellabitta deleted the local-testing-fixes branch March 31, 2025 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ellipsis-dev ellipsis-dev[bot] ellipsis-dev[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@mdellabitta